xds bootstrap: enable using JWT Call Credentials (part 2 for A97) #8536
Conversation
There was a problem hiding this comment.
Thanks for the feedback and apologies for the delay, @easwars , it's been a hectic week.
The PR is ready for a another look, PTAL.
internal/xds/bootstrap/bootstrap.go
Outdated
| // CallCreds returns the built call credentials that are ready to use. | ||
| // These are the credentials that were successfully built from the call_creds | ||
| // configuration. | ||
| func (sc *ServerConfig) CallCreds() []credentials.PerRPCCredentials { | ||
| return sc.selectedCallCreds | ||
| } |
There was a problem hiding this comment.
No, we don't, other than for testing; I had it for symmetry with ChannelCreds.
I've assumed that you'd prefer that we delete it so I have done so and adjusted the unit tests a bit (in bootstrap_test.go and clientimpl_test.go). A downside is that DialOpts feels a bit distant/opaque when unit testing the call credentials config functionality.
| } | ||
| features := strings.Join(sc.serverFeatures, "-") | ||
| return strings.Join([]string{sc.serverURI, sc.selectedCreds.String(), features}, "-") | ||
| return strings.Join([]string{sc.serverURI, sc.selectedChannelCreds.String(), features, sc.CallCredsConfigs().String()}, "-") |
There was a problem hiding this comment.
Yeah, it's a bit verbose and why I was asking about it (I should have been clearer). The returned value looks like this xds-server:443-tls-{}-jwt_token_file-{"jwt_token_file":"/token.jwt"}
internal/xds/bootstrap/bootstrap.go
Outdated
| case !slices.Equal(sc.serverFeatures, other.serverFeatures): | ||
| return false | ||
| case !sc.selectedCreds.Equal(other.selectedCreds): | ||
| case !sc.selectedChannelCreds.Equal(other.selectedChannelCreds): |
There was a problem hiding this comment.
I agree - because we're already comparing sc.channelCreds and other.channelCreds and because the config doesn't get dynamically updated, it makes sense.
Shall I just remove it in this PR or send a follow-up PR in case we're missing something?
|
This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed. |
|
@dimpavloff : Sorry for the delay in getting back. I was away for a while. Thanks for taking care of all of my comments. I will make one more pass today and will assign it to someone else for a second review. |
There was a problem hiding this comment.
Thanks for the review and for the fixes @easwars ! I've addressed the latest comments.
4 participants
Part two for grpc/proposal#492 (A97), following #8431 .
What this PR does is:
internal/xds/bootstrapwith support for loading multiple PerRPCCallCredentials specifed in a newcall_credsfield in the boostrap file as per A97xds/internal/xdsclient/clientimpl.goto use the call credentials when constructing the clientxds/bootstrapto register thejwtcredscall credentials and make them available ifGRPC_EXPERIMENTAL_XDS_BOOTSTRAP_CALL_CREDSis enabledI have added
DialOptionsWithCallCredsForTransportbecause, even though current and future call credentials are likely to all expect secure transport, I thought it would be safer to check of insecure transport just in case. If you prefer, I can just updateDialOptionsto use all call credentials regardless of the transport.Relates to istio/istio#53532
RELEASE NOTES: